-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Automate conversion of options to arguments #70
Conversation
…d all flags should be separate entries in args array
This required doing away with not overriding --branch with --bare: The git CLI does not do this, so I do not think that it is our place to do it here
@@ -2,65 +2,78 @@ | |||
|
|||
var async = require('grunt').util.async; | |||
var grunt = require('grunt'); | |||
var ArgUtil = require('flopmang'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the new module that I created.
Really good stuff. The fact that the test suite keeps passing (aside from two minor details, but they're OK) despite this big change is a testament to the quality of it. I think you'll agree that this is a huge change, not something we can push as an update to Unless there's a good reason not to, I suggest we merge this to master and then work at ensuring #44 is fine, to lead up to a new big release. |
That sounds good. We can also make a non -production release, and ask users to test with their Grunt workflows in order to catch any bugs that might have been missed by the unit tests. |
I'm glad that you like it. I'm happy to help with #44, but adding all git options will be a tedious task. Would you be willing to split up the git commands? |
We don't need all of them, I only really care about throwing out the ones where we differ and keeping the ones we already support. Any missing flags can be added later on as future improvements. |
That sounds good. I will start working on #44 after this gets merged into master. |
Automate conversion of options to arguments
Merged! Fantastic work. Now that you've written most of the module (in its current state), would you be interested in co-maintaining it? I'll add you to the git repo, so you can commit to it directly for small fixes. Do send PRs for bigger changes / things you're unsure of (or need a second pair of eyes on). And just ping me to roll releases. Psyched, this is a big step forward for the module. |
Thanks @rubenv! I'm psyched as well. I am happy to accept your offer of co-maintaining this library, and will still continue to use PRs for most changes: I'm a firm believer in code review and audit trails. I'll try to keep each PR small from now on so that it is quick to review. |
👍 |
As discussed in PR #61, this pull request creates a more structured system for converting task-options to CLI flags and arguments. After I tweaked my original solution to accommodate the various use cases, I decided that it would be easiest to put the bulk of the logic in a separate module that is reusable by other projects.
This PR uses
flopmang
, a module which I wrote just for this purpose. It is fully unit-tested, and I encourage you to look over the code when you have a chance: https://github.com/dylancwood/flopmang.I tried to keep the changes here small in scope by not modifying any of the tests (except one, see notes in-line) or adding any new options. Hopefully this gives you high confidence that the changes do not affect the functionality of the code.
Once this is merged (when you approve, of course), it should be even easier for me and other collaborators to add more tasks and options to continue expanding this module's coverage of the git CLI.
Please let me know if you have any questions or concerns.